feat(vnext): index bounded CTE visibility - #188
Merged
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Introduces a new vNext, bounded (resource-limited) CTE layout + visibility index to improve relation completion correctness across multiple SQL dialect subsets, and updates the dialect runtime contract to support tri-state CTE identifier equality/prefix matching.
Changes:
- Add a private, bounded CTE layout builder (
analyzeSqlCteLayout) and visibility projection (visibleSqlCtesAt) with explicit “partial/unavailable” fail-closed behavior. - Extend vNext relation-completion runtime types to use tri-state CTE identifier comparison and prefix matching.
- Enhance bounded SQL lexer limit reporting with
resourceAt, plus add extensive tests/benchmarks and ADR updates.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/vnext-types/marimo-relation-completion.test-d.ts | Updates the d.ts contract test to the new tri-state CTE runtime APIs. |
| src/vnext/relation-completion-types.ts | Adds tri-state CTE comparison/prefix types and updates the dialect runtime interface accordingly. |
| src/vnext/cte-layout.ts | New bounded CTE layout + visibility implementation with resource limits and fail-closed behavior. |
| src/vnext/bounded-sql-lexer.ts | Adds resourceAt to pinpoint where lexer resource limits were hit. |
| src/vnext/tests/cte-layout.test.ts | Adds comprehensive unit tests for CTE layout/visibility correctness and boundary conditions. |
| src/vnext/tests/cte-layout.bench.ts | Adds vitest benchmarks for representative CTE layout workloads. |
| src/vnext/tests/bounded-sql-lexer.test.ts | Extends lexer tests to validate resourceAt behavior at resource boundaries. |
| docs/adr/0005-parser-independent-relation-completion.md | Updates ADR with the bounded grammar matrix, tri-state identifier comparison, and CTE visibility semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bounds and performance
Representative local means:
Verification
8a1572369a171f49d3e23aeed4993aceb88a2cb2Deferred follow-up
The bounded grammar currently authenticates
SELECTquery leaders. Dialect-owned support for PostgreSQLVALUES/data-modifying CTE bodies, DuckDBFROM-first queries, and additionally parenthesized BigQuery recursive terms remains an explicit follow-up before relation completion is feature-complete.Part of #169.
Summary by cubic
Adds a bounded CTE layout and visibility index for vNext to improve relation completion across PostgreSQL, DuckDB, BigQuery, and Dremio, and now rejects invalid CTE cursor positions to avoid bogus suggestions. Introduces tri-state CTE identifier comparison and strict, fail-closed resource limits; part of #169.
New Features
visibleSqlCtesAt; now rejects invalid cursor positions and the lexer reports limit hits withresourceAt, failing closed on malformed input.Migration
SqlRelationCompletionDialectRuntime, replacecteIdentifiersEqualwithcompareCteIdentifiersreturning"equal" | "distinct" | "unknown".cteIdentifierMatchesPrefixreturning"match" | "no-match" | "unknown".Written for commit 7302de4. Summary will update on new commits.